There's a fair amount of code added here. Was there a particular place (or handful of places) you looked for working out what this code should look like? That'll be helpful to point to in commit messages.

For example, specific files in the legacy app; or places in the Flutter tree; or Android documentation; or other sources.

Can this method (and its helper getIntentSharedFileInfo) move onto a class other than MainActivity? Perhaps on AndroidIntentEventListener, since that seems closely related.

That would help for making it explicit what information or methods they're using, if any, from FlutterActivity or its base classes.

Even if the methods have to be passed this as a FlutterActivity (or some supertype of that), I think the explicitness of accessing FlutterActivity members on that argument would be helpful.

Agreed, this would be a helpful bit of code organization.

I think a good home for that would be in lib/widgets/share.dart . It's fundamentally closely tied to the SharePage widget.

Let's also make this callback a named function. Can be a private static method on the relevant class.

How about inlining this at the call site? The caller can use the controller getter, which is already exposed in the ComposeBoxState interface.

(That's effectively what we do for things like quote-and-reply.)

Not sure if this TODO is just a note to yourself or if you'd like input on it — if the latter, I'd be curious to hear what behavior you're seeing, and what behavior you expected 🙂

Similar comment as for the onSelect on _NewDmHeader above.

Interesting. What goes wrong without this change, if the user does try to open a channel action sheet?

The issue was that user could open the Topic List page from that action sheet, and when selected a topic from that topic list, the compose box of the topic message list wouldn't be populated with the content.

See discussion: #mobile-design > share to Zulip @ 💬

The latest revision now replaces this flag with one to disable just the topic list button from the channel action sheet, rather than disabling whole action sheet.

That sounds potentially useful but definitely a follow-up task, if this version already seems to work correctly 🙂

Does this need the else? It seems like this could work just fine with both if blocks acting independently.

Probably then put the text before the uploads.

I'm imagining that there might be sharing sources that include both some text and some images or other files. I'm not sure if the platform APIs support that; but it's definitely a natural thing for people to want to do, so perhaps they might. (After all, even when the thing shared from the other app is purely an image, people often/usually want to add to add some text before sending it, in Zulip as well as in many other apps.)

I think specifically that means this latter step should happen before doing an await on the former step.

Thanks! Fixed in current revision, and also updated videos, PTAL.

compose [nfc]: Expose uploadFiles on ComposeBoxState

This will be used soon to allow uploading files directly
via using `MessageListPageState`.

I don't love this change to _uploadFiles's interface: on the surface, it looks like contentController and contentFocusNode disagree about whether the content input always or only sometimes exists.

From the later commit, I can infer that this change isn't actually about the presence/absence of the content input; it's about a detail of how the function interacts with the input. Instead of doing this, how about making a flag that's specific to that behavior detail, like bool shouldRequestFocus = true?

Relatedly (but I wouldn't block on it), _uploadFiles could get its contentController and contentFocusNode from the ComposeBoxController, couldn't it? That could be a single param to replace those two params. Or it could even come from the context param, via ComposeBoxInheritedWidget.of, with some prep work to add a field for it to ComposeBoxInheritedWidget.

nit: s/compose box/content input/

nits:

• "There is" instead of "There was", right?
• "log in" for the verb, as here; "login" for the noun

How about composeBoxController.requestFocusIfUnfocused() for this? 🙂

How about adding TODO(#412) add onTopicSelect? In share-to-Zulip, that will probably be more useful to people, when we get around to it :) especially if it includes a "new topic" button as I've just suggested on that issue: #412 (comment)

…Actually, trying this out now, I think we might (eventually) want to force the user to choose a topic or start a new topic. When I land on the interleaved channel view with an empty topic input, the first thing I tried was to tap a topic recipient header, which brought me to a new page (the topic page) without the content I wanted to share.

nit: How about:

• naming this IntentEventListener or even AndroidIntentEventListener, corresponding to AndroidIntentsEventChannelApi.androidIntentEvents()

• and below, rename onAndroidIntentEvent to just onEvent?

This way feels more streamlined in loading the "event" abstraction into the reader's brain, and also making it clear that "android intent event" is the only kind of event this class is responsible for.

I think this simplifies to the following:

• Comment that the doc is wrong to imply that EXTRA_TEXT and EXTRA_STREAM can't both be present ("get*Extra can have either a EXTRA_TEXT or EXTRA_STREAM field"); this happens empirically
• First, check for EXTRA_STREAM. If it's present, include the file(s).
• Add EXTRA_TEXT, too, if that's present. No mime-type check (the current revision defeats sharing a plain-text file).
• Maybe a comment that EXTRA_TEXT might be text that the user doesn't want, like the URL of an image on the web. But, shrug, we're including it anyway because that's what Chrome/etc. put in the intent, and it's the other app's responsibility to choose what it intends to be shared.

Is that right?

Correct. Also, sent a new revision further simplifying and eliminating the if ("text/plain" == intent.type) { branch, PTAL.

Do we want to call lookupMimeType when sharedFile.mimeType is null? (See example in compose_box.dart)

If that's complicated, we could file a followup for it.

It's nice to do a tearoff for _handleNarrowSelect, i.e. onChannelSelect: _handleNarrowSelect vs. onChannelSelect: (…) => _handleNarrowSelect(…). But is that the only reason that _handleNarrowSelect is getting passed a BuildContext from much deeper than this in the widget tree?

If _handleNarrowSelect adds some async work up front (which, true, it doesn't have right now), then we'll need an isMounted check after the await…and then I think we need to investigate if something like a channel-rename can cause the context to unmount, if the context is on a per-channel widget.

Simpler to just use this widget's context, if we can:

onChannelSelect: (narrow) => _handleNarrowSelect(narrow, context)

right?

Hmm, I had added the BuildContext because in the initial draft I was observing a bug that pressing back from the message list page, when opened from new dm conversation action sheet. I would be led to the share page again instead of the home page. Even with the extra Navigator.pop for the action sheet. And I had thought that adding the BuildContext and passing that for Navigator was what fixed it.

But for some reason after I've removed this BuildContext now, I do not observe that bug again, so looks like it was something else in the initial draft that was not working.

Anyway removed the BuildContext in the latest revision.

This calls for a

          // Don't pad the bottom here; we want the list content to do that.
          bottom: false,

. And if we're settled on repurposing SubscriptionListPageBody and RecentDmConversationsPageBody¹, then a few more changes are needed:

• Revert the changes to those widgets in 742320c
• Move the "New DM" button up by the height of the bottom inset (the new-DM button landed after we added the bottom nav)
• Find a factoring or comment to explain why the FooPageBody widgets aren't uniform in these details

I think the "hide channels/conversations where the user can't post" flags, and the "don't offer channel action sheet" flag, are definitely signs that these existing "page body" widgets are probably not the abstractions we'd ideally want to be sharing, and that the ideal way to factor this would probably involve two new widgets and a certain amount of duplication.

OTOH there's quite a bit of logic in both of these pages, and the bulk of it I wouldn't want to duplicate: we really do mean for the the bulk of that behavior and UI to be uniform between this sharing context and the home-tabs context, and would risk divergence if we started having two copies of it. I think getting to the point where we could duplicate the right bits without duplicating all the rest would require some nontrivial refactoring.

So I think repurposing those widgets is the right solution for the present.

For the SafeArea: the page-body widgets are already taking care of that, right? As Chris says, those need bottom: false added. Then they'll continue to take care of the left and right insets; and the top inset is handled by the app bar.

So I think this SafeArea can be omitted. Is there something that goes wrong if it's left out?

For the SafeArea: the page-body widgets are already taking care of that, right?

Taking care of what specifically? 🙂

In this revision, in the emulator I'm running, the middle of the list looks like this:

No change if I remove this SafeArea.

It should look like this, in the middle of the list:

And this, at the bottom of the list:

But it doesn't because 742320c stopped delegating to list content the job of detecting and padding the bottom inset, and instead relied on the assumption that that's handled externally, by the bottom tabs.

Right. So the page-body widgets are already taking care of the left and right insets (and, wrongly for this purpose, a bottom inset). There's nothing for a SafeArea here to do.

And those other SafeArea calls need to be adjusted to not try to handle a bottom inset.

Sent this as c9391df, PTAL.

Responded there—for some reason, as Greg pointed out, my comment didn't end up showing on the PR thread: c9391df#r163401974

Following up from #1774 (comment) , this comment is still confusing to me:

• It makes it sound like the doc implies that the EXTRA_TEXT string itself represents a mime type, but the doc doesn't imply that. (It mentions getType giving a mime type.)
• About the possibility that EXTRA_TEXT and EXTRA_STREAM are both present:
  • It should make clear why we believe this behavior is possible: because we've seen it. (IOW it's not from reading code or seeing a StackOverflow thread or something)
  • It should make clear why that's interesting/important: because it contradicts the Android doc, and we've created our own interpretation of what it means.
  • It would be helpful to make clear what interpretation we're relying on: that files and text are both included in the share-to intent, not just one or the other.

How about:

    assert(
      intentAction == Intent.ACTION_SEND
          || intentAction == Intent.ACTION_SEND_MULTIPLE
    )

    // EXTRA_TEXT and EXTRA_STREAM are the text and file components of the content, respectively.
    // The ACTION_SEND{,_MULTIPLE} docs say "either" / "or" will be present:
    //   https://developer.android.com/reference/android/content/Intent#ACTION_SEND
    // But empirically both can be present, commonly, so we accept that form, interpreting it as
    // an intent to share both kinds of data.
    //
    // Empirically, sometimes EXTRA_TEXT isn't something we think needs to be shared, like the URL 
    // of a file that's present in EXTRA_STREAM…but we shrug and include it anyway because we don't
    // want to second-guess other apps' decisions about what to include; it's their responsibility.

    val extraText = intent.getStringExtra(Intent.EXTRA_TEXT)
    val extraStream = when (intentAction) {
      Intent.ACTION_SEND -> {
        // etc.

In this comment, could we mention the idea of forcing the user to choose a topic or start a new topic? (Or maybe somewhere more specific to the share-to code.) As I mentioned in https://github.com/zulip/zulip-flutter/pull/1774/files#r2248956083 , the current behavior can be pretty frustrating:

When I land on the interleaved channel view with an empty topic input, the first thing I tried was to tap a topic recipient header, which brought me to a new page (the topic page) without the content I wanted to share.

(It's OK if the intent doesn't have both things, but not OK if it has zero of them.)

nit: key is for the widget, so put it next to the other widget arguments

Cleanest to use a single timestamp (a single byDate value) through the whole list.

nit: indent arguments

This logic doesn't implement all the nuances that the name suggests it would.

That's fine for now, but it should get a TODO comment — pointing to #791.

nit:

Otherwise it reads like it's a "DMs action sheet" which is new.

nit: this name risks confusion, because this method is only sometimes the way that this widget handles selecting a channel.

Good alternatives would be:

• name it with "default", as in _defaultHandleChannelSelect
• or move the fallback logic inside this method (like _RecentDmConversationsPageBodyState does) so that the method's role matches its current name